Skip to content

Conversation

@bkruczyk
Copy link
Contributor

No description provided.

@josteink
Copy link
Member

There's always weird edge-cases when parsing is involved.

That said, the code in question here has been unchanged since the initial commit, with none of the current maintainers altering it.

For me at least, that means I don’t know any specific reasons to keep it as is, in its current form.

I’d be happy to accept this PR if you can:

  1. Prove it fixes a bug and
  2. Don’t break anything else.

And the best way to do that would be adding some test-cases to the test-suite for regexp-parsing.

Take a look at the existing tests and let us know if you need any help.

@bkruczyk
Copy link
Contributor Author

@josteink thanks for feedback, maybe we can discuss test cases when I will update the PR, need a day or two

@josteink
Copy link
Member

Sure thing. Take your time.

@bkruczyk bkruczyk force-pushed the fix-regex-font-locking branch from 64945c7 to f11490f Compare October 22, 2017 12:10
@bkruczyk
Copy link
Contributor Author

If anyone have anything to add on added tests, you're welcome to leave a comment :)

@josteink
Copy link
Member

Thanks for the update!

When it comes to unit-tests I usually check one thing before considering myself "happy": I ensure that the test prove the error-condition by failing before the appropriate fix is applied.

In this case your test are equally good before and after your fix is applied, meaning they are not sufficient to reproduce the error you say you want fixed.

If you could you address that (that is, make the tests fail without your fix in place), I'll consider this good and will be happy to merge!

@bkruczyk bkruczyk force-pushed the fix-regex-font-locking branch from f11490f to 393ac6d Compare October 22, 2017 14:53
@bkruczyk
Copy link
Contributor Author

That's a great tip on testing "fixes", I'll incorporate it in my daily work, thanks!
I've updated the PR.

@josteink
Copy link
Member

josteink commented Oct 22, 2017

Now that's more like it! Now the tests behave as tests should :)

And given my promise just one comment away, sorry about being a bit thick-headed here today (got a cold and all), but had the impression that this PR was meant to fix either #34 or #44.

As far as I can see from my interactive testing that's not the case though. Or is there something else I have misunderstood? Was this PR mostly about simplifying existing code?

If I try the testcase from #34:

test.replace(/\/g,"/");
this_identified_is_still_a_string

Second line is still highlighted as a string somehow. Is there something wrong with my test-setup perhaps?

Again: Sorry for being thick-headed about all this when you're obviously putting in a good effort just trying to full-fill my requests.

Edit: If nothing else, we can at least be happy Github too has the fontification of this expression wrong :)

@bkruczyk
Copy link
Contributor Author

Yup I intended for this PR to be a fix to #34.

I think the highlighting is correct, please check this comment :)

@josteink
Copy link
Member

Ah gotcha. My bad.

Thanks for your contribution and consider this merged!

@josteink josteink merged commit 56a1ea8 into emacs-typescript:master Oct 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants